-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle responses as JSON only if required to. #175
Conversation
A JSON response will have "application/json", and only if this is set should the response content be decoded as such. Otherwise (until something smarter comes along), on might as well return the raw response content (which happens to be the correct thing to do for "text/plain" content).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! This change looks sensible to me.
content_value = response.json() | ||
content_type = response.headers.get('content-type', '').lower() | ||
|
||
if content_type.startswith(APP_JSON): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you went with startswith instead of equality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To opportunistically capture "application/json-rpc" should that ever occur? Admittedly this test may need to evolve over time since there are various other content types that could contain json. I don't feel strongly about this either way.
This might be a breaking change for users where the server isn't sending the correct content type header. I'm wondering if we should make this behavior configurable, i.e. add an option to make bravado-core behave the way it used to. The other option would be to ship this as-is and wait for user feedback. Thoughts? |
My approach here is to KISS. And yes in the future there will be some
breakage. But there's arguably more value in detecting the breakage,
understanding the variety of non-compliant systems and putting in explicit
fixes. The alternative is that you'd never be sure as to whether the
configuability you've added preemptively is ever used and therefore whether
you could ever change it - since you don't know how/if it is being used.
For that reason I don't think the existing code (which assumed JSON
content) was a bad idea - it was good enough until the time it proved to be
insufficient and is now being updated because of a real need.
…--
Michal Ostrowski
On Tue, Jun 6, 2017 at 6:45 AM Stephan Jaensch ***@***.***> wrote:
This might be a breaking change for users where the server isn't sending
the correct content type header. I'm wondering if we should make this
behavior configurable, i.e. add an option to make bravado-core behave the
way it used to. The other option would be to ship this as-is and wait for
user feedback. Thoughts?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#175 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALnLD1jVeWs8EGzZE9G5ky5BEkTq7jgpks5sBTvigaJpZM4NsC-G>
.
|
Yeah, that sounds reasonable. Let's make sure we release this as a major version. |
A JSON response will have "application/json", and only if this is set should
the response content be decoded as such. Otherwise (until something smarter
comes along), on might as well return the raw response content (which
happens to be the correct thing to do for "text/plain" content).